Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[release/5.0-rc2] Add JsonNumberHandling.AllowReadingFromString as a JsonSerializer web default (#41539) #42240

Merged
merged 1 commit into from
Sep 15, 2020

Conversation

layomia
Copy link
Contributor

@layomia layomia commented Sep 15, 2020

Backport of #41539 to release/5.0-rc2

Customer Impact

Follow up from #39363 / #30255.

During API review for the number handling feature, it was discussed that number types being written as strings is a common pattern in many JSON producers (e.g API endpoints) across the web. Adding JsonNumberHandling.AllowReadingFromString as a JsonSerializer web default makes it easier for .NET web applications to consume these numbers in a consistent manner (same options for client/shared/server).

Testing

Risk

The JsonSerializerDefaults and number handling features are new in .NET 5.0. .NET web products such as ASP.NET MVC, HttpRequestExtensions, and the extension methods on HttpClient/JsonContent have been updated to use JsonSerializerDefaults by default. The change in this PR affects these areas and makes reading numbers from JSON strings the default behavior.

… default (dotnet#41539)

* Add JsonNumberHandling.AllowReadingFromString as a JsonSerializer web default

* Add test assertion for number handling option in S.N.H.Json

* Test number-as-string behavior in System.Net.Http.Json
@layomia layomia added Servicing-consider Issue for next servicing release review area-System.Text.Json labels Sep 15, 2020
@layomia layomia added this to the 5.0.0 milestone Sep 15, 2020
@layomia layomia requested a review from jozkee as a code owner September 15, 2020 04:34
@layomia layomia self-assigned this Sep 15, 2020
@danmoseley
Copy link
Member

danmoseley commented Sep 15, 2020

Thanks for the detailed write up. This change seems like a feature. It will be hard to get the okay for it without strong customer motivation. Can you share more about the customers? How many or how large? How impactful if it to them if we don’t take this change eg they can’t migrate?

@danmoseley danmoseley closed this Sep 15, 2020
@danmoseley danmoseley reopened this Sep 15, 2020
@danmoseley
Copy link
Member

Hit close by mistake

@ahsonkhan
Copy link
Member

ahsonkhan commented Sep 15, 2020

This is changing the default settings of JsonContent and JsonSerializerDefaults.Web. If not done in 5.0, it will become a behavioral breaking change in 6.0 that will be difficult for app authors to workaround the break.

@danmoseley
Copy link
Member

Thanks @ahsonkhan . That's helpful but still we would need strong customer data to support the change.

@layomia
Copy link
Contributor Author

layomia commented Sep 15, 2020

@danmosemsft

The number handling feature is the most requested feature in System.Text.Json (both closed and open) with 91 likes. The next highest, the reference handling feature, has 77 likes. The major motivation of the feature was to support interchanging data with various web services including JavaScript APIs, Blazor, Python etc. The primary capability desired was being able to read numbers from JSON strings produced by these entities (JsonNumberHandling.AllowReadingFromString).

The setting of JsonNumberHandling.AllowReadingFromString when JsonSerializerDefaults.Web is specified allows this highly desired behavior to work without additional configuration in user projects. Also, since ASP.NET MVC, HttpClientExtensions/Blazor are using JsonSerializerDefaults.Web, users won't have to worry about specifying this behavior at multiple layers of their application stack (client, shared, server) and have potential bugs with leaving out options at one layer. This a pain-point experienced by some users, including @terrajobst on our team.

FWIW during API review for the number handling feature, there was even discussion about making this a general default on JsonSerializerOptions. It seemed a bit too general for low level serializer usages, so we settled on making it a web default where it is certainly more applicable.

If not done in 5.0, it will become a behavioral breaking change in 6.0

Just to clarify that this behavior is making a scenario more permissive, specifically going from throwing JsonException to successfully coercing a number from a JSON string. This is technically not a breaking change. While some people might rely on the strict behavior of the serializer by default, I imagine this subset to be small in web interchange contexts.

BTW, the breaking change angle here would be from the perspective of changing the behavior of a named default (JsonSerializerDefaults.Web), not from ASP.NET MVC etc defaults changing. The settings change from the perspective of ASP.NET and System.Http.Net.Json, while significant, was already agreed on in API review and further discussions with @terrajobst and @pranavkm, and is anticipated on their side.

will be difficult for app authors to workaround the break.

App authors can revert to strict settings by setting JsonNumberHandling.Strict on JsonSerializerOptions. If the setting was made in a library that the app depends on, hopefully the library author is looking out for updates in System.Text.Json or can be contacted to update the setting. Again I think this subset is small.

That said, I believe it is still worth it to make the change now to avoid a potential inconvenience to some users in 6.0. Specifically, this will be users that directly use JsonSerializerDefaults.Web and rely on strict number handling.

@danmoseley danmoseley removed the Servicing-consider Issue for next servicing release review label Sep 15, 2020
@danmoseley
Copy link
Member

Removing label while discussion continues. We can take Thurs if needed

@danmoseley danmoseley added the Servicing-consider Issue for next servicing release review label Sep 15, 2020
@leecow leecow added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Sep 15, 2020
Copy link
Member

@steveharter steveharter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM; direct port except for minor doc change that will be added to master.

@layomia layomia merged commit ab41ed8 into dotnet:release/5.0-rc2 Sep 15, 2020
@theolivenbaum
Copy link

My 2 cents: was also surprised by this when updating to use System.Text.Json. Number serialization in json has a few limitations, specially for anything outside the float range/precision, thus quite often one has to fall back to use strings instead

@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
@layomia layomia deleted the web_defaults_net5.0 branch May 18, 2021 07:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Text.Json Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants